migrate trio.ssl from star import to explicit imports, to aid static analysis issue #542 #752

jmfrank63 · 2018-10-28T00:04:56Z

Use less magic in constructing our public API exports, to make trio more intelligible to static analysis #542

jmfrank63 · 2018-10-28T00:06:22Z

One test is failing but I cannot see where the import gets overwritten again.

____________________________________________ test_module_metadata_is_fixed_up ____________________________________________

    def test_module_metadata_is_fixed_up():
        import trio
        assert trio.Cancelled.__module__ == "trio"
        assert trio.open_cancel_scope.__module__ == "trio"
>       assert trio.ssl.SSLStream.__module__ == "trio.ssl"
E       AssertionError: assert 'ssl' == 'trio.ssl'
E         - ssl
E         + trio.ssl

trio/tests/test_util.py:73: AssertionError

Any ideas are welcome.

codecov · 2018-10-28T00:54:40Z

Codecov Report

Merging #752 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #752      +/-   ##
==========================================
+ Coverage   99.01%   99.01%   +<.01%     
==========================================
  Files          96       96              
  Lines       11673    11677       +4     
  Branches      832      830       -2     
==========================================
+ Hits        11558    11562       +4     
  Misses         94       94              
  Partials       21       21

Impacted Files	Coverage Δ
trio/tests/test_exports.py	`96.36% <ø> (-0.07%)`	⬇️
trio/_ssl.py	`98.93% <ø> (-0.07%)`	⬇️
trio/tests/test_ssl.py	`97.75% <100%> (ø)`	⬆️
trio/ssl.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bc3b0b...774e1e8. Read the comment docs.

trio/ssl.py

njsmith · 2018-10-29T05:41:40Z

trio/ssl.py

+ get_default_verify_paths, Purpose, enum_certificates, enum_crls,
+ SSLSession, VerifyMode, VerifyFlags, Options, AlertDescription,
+ SSLErrorNumber
+ )


These don't need to be fake! It's a well-defined set of things that should always be available. So we can just do a regular old import without any try/except.

Some actually don't but others do. I tried to import them directly but the test failed on some versions of python and with windows. I decided to revert back to being them all fake since doing a subset one way and the rest the other is more confusing and IMHO not worth the extra lines of code. Is this fine with you?

njsmith · 2018-10-29T05:43:22Z

trio/ssl.py

+ "VerifyFlags", "Options", "AlertDescription", "SSLErrorNumber"
+ ]
+ }
+)


Once we switch to a regular import above, we can delete this.

njsmith · 2018-10-29T05:45:38Z

trio/_ssl.py

-
-for _name in _stdlib_ssl.__dict__.keys():
- if _name == _name.upper():
- _reexport(_name)


This code is like trio.socket: it re-exports all the uppercased symbols. So we need some similar handling: a fake try/import, plus moving this loop into trio/ssl.py. With this PR right now it looks like we aren't re-exporting the uppercased symbols at all?

Ups, I overlooked this totally, yes of course, this has to be addressed.

They are now imported, but see my comment below about external dependencies like ssl library and version.

njsmith · 2018-10-29T07:20:13Z

It might make sense to merge #756 before this, and then use #756's new tests instead of adding tests here.

njsmith · 2018-11-05T00:44:09Z

This also needs a merge from master to pick up the changes in #756. Plus addressing the other comments above, of course :-)

jmfrank63 · 2018-11-05T12:33:28Z

This also needs a merge from master to pick up the changes in #756. Plus addressing the other comments above, of course :-)

Right, I did this for #753 but totally forgot about this one. I'll also remove the extra imports. Might take a couple of days, I am really busy right now.

njsmith · 2018-11-05T20:48:06Z

No worries 👍

jmfrank63 · 2018-11-10T17:32:03Z

That's strange. I expected the test to fail, as I am only importing the ssl Mac symbols, so the jedi and pylint test for windows and linux should show some missing symbols. I'll checkout this repo on windows and see if it passes locally as well check which symbols are imported.
I also tried to import directly but only a subset of symbols was available on each system which made the tests to fail. Therefore I moved back to the try catch block which seems fine now.
@njsmith Let me know what you think.

jmfrank63 · 2018-11-10T18:41:09Z

I can confirm the tests did not catchOP_NO_RENEGOTIATION of the ssl module missing on windows. I assume this should be another issue, I'll try to get the missing symbols on linux via wsl.

njsmith · 2018-11-10T23:02:38Z

I think that the main thing that determines which symbols appear on a given Python is (a) which version of Python you're using, and (b) which version of OpenSSL this Python was built against. I don't think it varies across platforms, except that if you grab a random Windows build and a random MacOS build then they may well have been built against different versions of OpenSSL.

jmfrank63 · 2018-11-11T12:06:19Z

I think that the main thing that determines which symbols appear on a given Python is (a) which version of Python you're using, and (b) which version of OpenSSL this Python was built against. I don't think it varies across platforms, except that if you grab a random Windows build and a random MacOS build then they may well have been built against different versions of OpenSSL.

Understood, but that does not make it especially easier. Python can not only be build against different versions of OpenSSL it can also be build against other SSL libraries, e.g. LibreSSL.
Building tests for these scenarios will IMHO be not worth the effort.

njsmith · 2018-11-24T09:33:44Z

It looks like this PR isn't actually enabling the generic exports test for the trio.ssl module? You need to remove this line from trio/test/test_exports.py:

NAMESPACES.remove("trio.ssl")

I see what you mean about the different versions being frustrating... but I'm still wincing a lot at the duplicate lists :-(. As one last try before we give up and maintain two lists of symbols, can we try the following?

# Always available
from ssl import (
     cert_time_to_seconds, CertificateError,
     create_default_context, DER_cert_to_PEM_cert,
     get_default_verify_paths, match_hostname,
     PEM_cert_to_DER_cert, Purpose, SSLEOFError, SSLError, 
     SSLSyscallError, SSLZeroReturnError
) 

# Added in python 3.6
try:
    from ssl import AlertDescription, SSLErrorNumber, SSLSession, VerifyFlags, VerifyMode, Options
except ImportError:
    pass

# Added in python 3.7
try:
    from ssl import SSLCertVerificationError, TLSVersion
except ImportError:
    pass

# Windows-only
try:
    from ssl import enum_certificates, enum_crls
except ImportError:
    pass

(It looks like the constants available depend on openssl version, but these lowercase things are mostly dependent on Python version.)

jmfrank63 · 2018-11-27T20:49:23Z

@njsmith Hi Nathaniel, thanks for the feedback and sorry for the delayed answer. I was really busy over the past days. I'll try to implement what you suggest and notify you once done. Thank you so much for letting me taking part of this awesome project.

…t skipping of trio.ssl

jmfrank63 · 2018-12-08T18:04:14Z

??? I thought the second last test failed for pypy but now it is actually telling me that this test succeeded?
Well the difference is just two blocks vs one but the browser behaviour sometimes is really weird.
The split of blocks can easily be fixed by a rollback though. @njsmith Could you take a look?

njsmith · 2018-12-12T02:18:48Z

This is looking good, except for some dead code left behind... let me try deleting that and check if it still works.

No semantic changes (I hope!)

migrated ssl towards static import

c64e9fa

Fixed failing test

d420472

jmfrank63 added 3 commits October 28, 2018 02:16

Removed obsolete outcommented code from _ssl.py

b873033

Added comments to ssl.py

acd9056

Merge branch 'master' into migrate_ssl_issue_542

18251cb

jmfrank63 mentioned this pull request Oct 28, 2018

Use less magic in constructing our public API exports, to make trio more intelligible to static analysis #542

Open

jmfrank63 added 2 commits October 28, 2018 15:00

Fixed fake explicit import from incorrect module

c8dc54e

Added ssl exports tests

e544bf8

njsmith reviewed Oct 29, 2018

View reviewed changes

jmfrank63 added 3 commits November 8, 2018 18:43

Merged master to get universal test

73aa156

Removed unnecessary fake imports

be73079

Added uppercase symbols

cc3469e

Added OP_NO_RENEGOTIATION symbol and sorted ssl imports

c45a8e0

jmfrank63 added 3 commits December 8, 2018 16:56

Modified imports to be more detailed towards versions and removed tes…

543f283

…t skipping of trio.ssl

Added missing ssl symbols for linux

f12275a

Moved missing pypy symbols into its own block

0b7a515

Merge branch 'master' into migrate_ssl_issue_542

9382c7f

jmfrank63 changed the title ~~migrated trio.ssl from star import to explicit imports, to aid static analysis~~ migrated trio.ssl from star import to explicit imports, to aid static analysis issue #542 Dec 9, 2018

jmfrank63 changed the title ~~migrated trio.ssl from star import to explicit imports, to aid static analysis issue #542~~ migrate trio.ssl from star import to explicit imports, to aid static analysis issue #542 Dec 9, 2018

Merged symbol try except blocks again

25a8fd7

njsmith added 2 commits December 11, 2018 18:22

Remove dead code and rearrange a bit

b37f20a

No semantic changes (I hope!)

Add back import I accidentally deleted

774e1e8

njsmith merged commit 9799564 into python-trio:master Dec 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate trio.ssl from star import to explicit imports, to aid static analysis issue #542 #752

migrate trio.ssl from star import to explicit imports, to aid static analysis issue #542 #752

jmfrank63 commented Oct 28, 2018

jmfrank63 commented Oct 28, 2018

codecov bot commented Oct 28, 2018 •

edited

Loading

njsmith Oct 29, 2018

jmfrank63 Nov 11, 2018

njsmith Oct 29, 2018

njsmith Oct 29, 2018

jmfrank63 Nov 8, 2018

jmfrank63 Nov 11, 2018

njsmith commented Oct 29, 2018

njsmith commented Nov 5, 2018

jmfrank63 commented Nov 5, 2018

njsmith commented Nov 5, 2018

jmfrank63 commented Nov 10, 2018

jmfrank63 commented Nov 10, 2018

njsmith commented Nov 10, 2018

jmfrank63 commented Nov 11, 2018

njsmith commented Nov 24, 2018 •

edited

Loading

jmfrank63 commented Nov 27, 2018

jmfrank63 commented Dec 8, 2018

njsmith commented Dec 12, 2018

migrate trio.ssl from star import to explicit imports, to aid static analysis issue #542 #752

migrate trio.ssl from star import to explicit imports, to aid static analysis issue #542 #752

Conversation

jmfrank63 commented Oct 28, 2018

jmfrank63 commented Oct 28, 2018

codecov bot commented Oct 28, 2018 • edited Loading

Codecov Report

njsmith Oct 29, 2018

Choose a reason for hiding this comment

jmfrank63 Nov 11, 2018

Choose a reason for hiding this comment

njsmith Oct 29, 2018

Choose a reason for hiding this comment

njsmith Oct 29, 2018

Choose a reason for hiding this comment

jmfrank63 Nov 8, 2018

Choose a reason for hiding this comment

jmfrank63 Nov 11, 2018

Choose a reason for hiding this comment

njsmith commented Oct 29, 2018

njsmith commented Nov 5, 2018

jmfrank63 commented Nov 5, 2018

njsmith commented Nov 5, 2018

jmfrank63 commented Nov 10, 2018

jmfrank63 commented Nov 10, 2018

njsmith commented Nov 10, 2018

jmfrank63 commented Nov 11, 2018

njsmith commented Nov 24, 2018 • edited Loading

jmfrank63 commented Nov 27, 2018

jmfrank63 commented Dec 8, 2018

njsmith commented Dec 12, 2018

codecov bot commented Oct 28, 2018 •

edited

Loading

njsmith commented Nov 24, 2018 •

edited

Loading